Skip to content

feat: Enhance ArgoCD CLI: Dynamic Repo Server Retrieval with --core and --refresh Flags#17613

Merged
ishitasequeira merged 19 commits intoargoproj:masterfrom
Mangaal:argocd-cli-core-flag
Mar 29, 2024
Merged

feat: Enhance ArgoCD CLI: Dynamic Repo Server Retrieval with --core and --refresh Flags#17613
ishitasequeira merged 19 commits intoargoproj:masterfrom
Mangaal:argocd-cli-core-flag

Conversation

@Mangaal
Copy link
Member

@Mangaal Mangaal commented Mar 25, 2024

Description

When using the ArgoCD CLI client with the --core and --refresh flags, it's essential to provide the repo-server-name argument for the default ArgoCD instance deployed with operators. However, in some cases, users might forget to specify this argument, leading to errors.

Current Behavior:
Running the command argocd app list --core or argocd admin app get-reconcile-results --refresh text.txt without providing the repo-server-name flag results in a default value of argocd-repo-server. This default value is not valid for ArgoCD installations with an operator. The actual repo server name is derived from the ArgoCD CR name, such as example-argocd-repo-server if the CR name is example-argocd. Consequently, an error occurs due to the mismatch between the default and actual repo server name.

How to Reproduce the Issue:
To reproduce the issue:

1. Install ArgoCD with an operator.
2. Configure the ArgoCD CustomResource (CR) name, e.g., example-argocd.
3. kubectl config set-context --current --namespace <some-namespace-where-argocd-is-deployed>
4. Execute the `argocd app list --core` command without specifying the repo-server-name flag.

You will encounter the following issue:

FATA[0000] cannot find pod with selector: [app.kubernetes.io/name=argocd-repo-server] - use the --{component}-name flag in this command or set the environmental variable (Refer to https://argo-cd.readthedocs.io/en/stable/user-guide/environment-variables), to change the Argo CD component name in the CLI

Fix:

This pull request enhances the ArgoCD CLI by dynamically fetching the correct repo server name instead of relying solely on a default value. It queries the ArgoCD component services with the label app.kubernetes.io/component=repo-server and updates the repo server name accordingly. If the service query fails, it falls back to the default value. This ensures compatibility with ArgoCD installations using operators and improves overall reliability.

Additionally, this pull request addresses a bug documented in issue #17602

Output after fix:

image

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Mangaal added 3 commits March 25, 2024 15:56
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
…rver labels , if the label exist construct label selector PortForward

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
@Mangaal Mangaal requested a review from a team as a code owner March 25, 2024 14:15
repoServerPodLabelSelector := common.LabelKeyAppName + "=" + c.repoServerName
repoServerName := c.repoServerName
repoServererviceLabelSelector := common.LabelKeyComponentRepoServer + "=" + common.LabelValueComponentRepoServer
repoServerServices, err := c.kubeClientset.CoreV1().Services(c.namespace).List(context.Background(), v1.ListOptions{LabelSelector: repoServererviceLabelSelector})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of deriving the argocd instance name from the services, can we not apply the label app.kubernetes.io/component=repo-server to the spec.template.metadata section of the repo server deployment and then use this label to get the right repo server.
https://github.com/argoproj/argo-cd/blob/f87897c53c6f04426953f5d3ca781d3240186a60/manifests/base/repo-server/argocd-repo-server-deployment.yaml#L16C9-L16C51

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anandf, I will update the implementation and add the labels "app.kubernetes.io/component=repo-server" to repo-server replica pods in the manifest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to app.kubernetes.io/component=repo-server component based label might cause confusion when customer provides argument --repo-server-name with the name of the argocd instance argocd-repo-server. So let's keep the logic of deriving the argocd repo server name from the service name.

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit regarding the PR's title: Can you please try to find a shorter, more concise title that denotes what actually has been fixed or improved? :)

@Mangaal Mangaal changed the title feat: ArgoCD CLI client in core mode requires repo-server-name arg for default ArgoCD instance for ArgoCD deployed with operators feat: ArgoCD CLI client in core mode requires repo-server-name arg Mar 26, 2024
@Mangaal Mangaal changed the title feat: ArgoCD CLI client in core mode requires repo-server-name arg feat: ArgoCD CLI client in core mode requires repo-server-name flag Mar 26, 2024
@Mangaal Mangaal force-pushed the argocd-cli-core-flag branch from 439f521 to 3b060b4 Compare March 26, 2024 13:19
@Mangaal Mangaal changed the title feat: ArgoCD CLI client in core mode requires repo-server-name flag feat: Enhance ArgoCD CLI: Dynamic Repo Server Retrieval with --core and --refresh Flags Mar 27, 2024
}

if !c.canHandleCluster(cluster) {
if !c.canHandleCluster(cluster) && c.clusterSharding == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the nil check should come first isn't it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anandf, I have updated the code to handle nil ptr dereference separately

Copy link
Member

@anandf anandf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

overrides := clientcmd.ConfigOverrides{}
repoServerPodLabelSelector := common.LabelKeyAppName + "=" + clientOpts.RepoServerName
repoServerName := clientOpts.RepoServerName
repoServererviceLabelSelector := common.LabelKeyComponentRepoServer + "=" + common.LabelValueComponentRepoServer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: repoServerServiceLabelSelector

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ishitasequeira, Updating the same.

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mangaal Just 1 nit. Rest looks good to me!!

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
@Mangaal Mangaal requested a review from a team as a code owner March 29, 2024 05:22
@ishitasequeira ishitasequeira merged commit 766a6da into argoproj:master Mar 29, 2024
mkieweg pushed a commit to mkieweg/argo-cd that referenced this pull request Jun 11, 2024
…nd --refresh Flags (argoproj#17613)

* add const key value for ComponentRepoServer

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* update NewRepoServerClient() to look for service with  ComponentRepoServer labels , if the label exist construct label selector PortForward

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* add comment for the new constants

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* instead of passing nil which leads to  nil ptr referance error, pass empty ClusterSharding{}

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* check for operator install repo server name

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* handle empty nil ptr dereference error

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* handle  nil prt dereference

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* typo correction

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* run clidocsgen

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

---------

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
…nd --refresh Flags (argoproj#17613)

* add const key value for ComponentRepoServer

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* update NewRepoServerClient() to look for service with  ComponentRepoServer labels , if the label exist construct label selector PortForward

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* add comment for the new constants

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* instead of passing nil which leads to  nil ptr referance error, pass empty ClusterSharding{}

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* check for operator install repo server name

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* handle empty nil ptr dereference error

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* handle  nil prt dereference

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* typo correction

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

* run clidocsgen

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>

---------

Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants